Free pages which become empty during a delete - #5771
Draft
gefjon wants to merge 9 commits into
Draft
Conversation
This commit expands upon PR 5770, causing empty pages to transition to `None` or the all-zeroes hash in two circumstances: 1. When a delete causes a page to become empty. 2. When a page is empty while loading from a snapshot. Historical snapshots are not rewritten; all-zeroes page hashes will appear only in newly-created snapshots. The diff here is relatively inflated due to now needing a `PagePool` in delete operations, in order to return freed pages to the pool. Many `Table` operations which one might not initially expect to perform a delete require this, as many operations are implemented in terms of transient inserts which are quickly deleted. Also, during implementation, I removed some `Pages` or `Table` operators which no longer make sense, as they cause a `Pages` to contain an empty `Page` at rest. These were only used by tests, which I have rewritten to not require them, and benchmarks, which had already bitrotted so significantly as to not be worth maintaining. I removed benchmarks sufficient to get `cargo check --tests --benches` passing, but did not attempt to repair `cargo test --benches`, as that appears to have been broken prior to this change.
…phoebe/free-empty-pages/free-during-delete
…phoebe/free-empty-pages/free-during-delete
2 tasks
…phoebe/free-empty-pages/free-during-delete
Contributor
Author
|
This is marked as draft because we should not merge it until the previous PR, #5770 , is released, deployed and stable, but it is otherwise ready for review. |
…phoebe/free-empty-pages/free-during-delete
Per Joshua's review, this commit adds a new metric, `spacetime_replay_snapshot_num_absent_pages`. Pages which aren't read from files due to having the all-zeroes hash are counted towards that metric and not towards the existing `spacetime_replay_snapshot_num_objects_read`.
…phoebe/free-empty-pages/free-during-delete
onx2
pushed a commit
to onx2/SpacetimeDB
that referenced
this pull request
Aug 22, 2026
# Description of Changes Preparation for freeing empty pages. Because the snapshot format depends on the density of page vectors and didn't previously reserve a sentinel, to preserve rollback safety we have to do this preparation step before actually implementing freeing pages as part of the row delete operation. In this PR, the `Table`/`Pages` switches to a `Vec<Option<Box<Page>>>`, with pages allowed to be absent. However, until a later patch, outside of tests, no page entry will ever be `None`. The table code is still able to use and reason about `None` page entries, as they may arise if we deploy said later patch, free a page, capture a snapshot, then roll back to this version. In the snapshot format, absent pages are recorded in the pages vec as the all-zeroes hash. Page objects are not written or read in this case; the all-zeroes hash does not correspond to an actual object on disk. When allocating a new page, we attempt to fill the lowest empty slot. We do this in log time by storing a `BTreeSet` of the empty slots, and popping the lowest value from it to use as the slot for the newly allocated page. I believe that for at least some access patterns, this should allow us to gradually converge on a dense array of pages in the case where rows are deleted at a higher rate than new inserts. As a result of this PR, some operators have changed in such a way as to cause the `table` crate benchmarks to no longer compile. These benchmarks appear to have already bitrotted significantly. I removed benchmarks sufficient to get `cargo check --tests --benches` passing, but did not attempt to repair `cargo test --benches`, as that appears to have been broken prior to this change. Per Joshua's review, this PR also adds a new metric, `spacetime_replay_snapshot_num_absent_pages`. Pages which aren't read from files due to having the all-zeroes hash are counted towards that metric and not towards the existing `spacetime_replay_snapshot_num_objects_read`. # API and ABI breaking changes Changes the snapshot format on-disk to recognize a new special sentinel, the all-zeroes hash. When the all-zeroes hash appears in a table's vector of page hashes, it means that no page exists in that slot, and so no object file is read. As of this PR, it is (or at least, should be) impossible to reach the added codepaths or representation without using a snapshot created by a newer version of SpacetimeDB or through manual editing. # Expected complexity level and risk 3: if it is mistakenly possible to put `None` in a `Pages` within this PR, then the release that introduces this commit may not be rollback-safe. Also, this touches the datastore, which contains `unsafe` code, though no new unsafe code is introduced, nor is any of it modified in any ways that would affect safety invariants. # Testing <!-- Describe any testing you've done, and any testing you'd like your reviewers to do, so that you're confident that all the changes work as expected! --> - [x] A couple new automated tests in the table crate. - [x] Manually tested rollback safety by using clockworklabs#5771 to construct a snapshot with an absent page, then used this PR to replay it. Described in more detail in comment below. --------- Co-authored-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
This commit expands upon PR #5770, causing empty pages to transition to
Noneor the all-zeroes hash in two circumstances:Historical snapshots are not rewritten; all-zeroes page hashes will appear only in newly-created snapshots.
The diff here is relatively inflated due to now needing a
PagePoolin delete operations, in order to return freed pages to the pool. ManyTableoperations which one might not initially expect to perform a delete require this, as many operations are implemented in terms of transient inserts which are quickly deleted.Also, during implementation, I removed some
PagesorTableoperators which no longer make sense, as they cause aPagesto contain an emptyPageat rest. These were only used by tests, which I have rewritten to not require them, and benchmarks, which had already bitrotted so significantly as to not be worth maintaining. I removed benchmarks sufficient to getcargo check --tests --benchespassing, but did not attempt to repaircargo test --benches, as that appears to have been broken prior to this change.This PR causes #5770 to no longer be safe to roll back. We will need to release and deploy the two PRs separately, and leave sufficient time following the deployment of #5770 to become confident that it is stable and will not need to be rolled back.
API and ABI breaking changes
Causes newly-written snapshots to use a sentinel to mark not-present pages, meaning that builds of SpacetimeDB prior to #5770 will be unable to load those snapshots.
Expected complexity level and risk
3: rollback safety concerns, and interacts with some
unsafecode in the datastore. No unsafe code is significantly modified, nor does this change interact in any way with any safety invariants, but it's still worth checking.Testing
Tableand snapshots #5770 , described in detail on that PR. This included capturing a snapshot with absent pages.